[CSTACKEX-216] Review Comments for Igroup Name Length#79
Conversation
suryag1201
requested review from
piyush5netapp,
rajiv-jain-netapp and
sandeeplocharla
as code owners
There was a problem hiding this comment.
Pull request overview
This PR updates the ONTAP igroup naming logic (and its unit tests) to change the generated name format, likely to better handle the 96-character igroup name limit by placing the host UUID earlier in the string.
Changes:
- Changed igroup name format from
cs_<svmName>_<hostUuid>tocs_<hostUuid>_<svmName>inOntapStorageUtils#getIgroupName. - Updated
OntapStorageUtilsTestexpected values and length-calculation comments to match the new format.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java | Changes igroup name construction order and updates the in-code format comment. |
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java | Updates unit tests to assert the new igroup naming format and related length assumptions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static String getIgroupName(String svmName, String hostUuid) { | ||
| //Igroup name format: cs_svmName_hostUuid | ||
| //Igroup name format: cs_hostUuid_svmName | ||
| String sanitizedHostUuid = hostUuid.replaceAll("[^a-zA-Z0-9_-]", "_"); | ||
| String igroupName = OntapStorageConstants.CS + OntapStorageConstants.UNDERSCORE + svmName + OntapStorageConstants.UNDERSCORE + sanitizedHostUuid; | ||
| String igroupName = OntapStorageConstants.CS + OntapStorageConstants.UNDERSCORE + sanitizedHostUuid + OntapStorageConstants.UNDERSCORE + svmName; | ||
| // ONTAP igroup names are limited to 96 characters; truncate if longer. |
There was a problem hiding this comment.
we will try to push this change in 4.23 so that we will not see this issue during upgrade
|
|
||
| public static String getIgroupName(String svmName, String hostUuid) { | ||
| //Igroup name format: cs_svmName_hostUuid | ||
| //Igroup name format: cs_hostUuid_svmName |
There was a problem hiding this comment.
For normal CloudStack UUIDs (hex + hyphens), sanitization is a no-op, so 550e8400-e29b-41d4-a716-446655440000 stays as-is. It only matters if the input has characters outside [a-zA-Z0-9_-].
Description
Issue: "getIgroupName() truncates the full "cs__" string as a whole. If svmName is long, this can truncate away most or all of the hostUuid portion, increasing the chance of igroup name collisions across different hosts (and also throws a NullPointerException if hostUuid is null). Consider validating inputs and truncating svmName first so the host UUID remains intact (or, if hostUuid itself is too long, truncate only the UUID)."
Fix: In typical deployments, truncation likely never triggers bcz total length will remain less than 96. But to avoid any issues for future where svm limits get exceeded, will change the igroup format to cs_hostUuid_svmName
Note: This is a breaking change for existing deployments. Igroups already on ONTAP use the old format (cs_svm1_hostname). After this change, CloudStack will look for cs__svm1 and may not find existing igroup
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
1- Created 1st VM on ISCSI Storage pool - Igroup got create with new name format attached a screen shot and lun got mapped
2- Create 2nd VM on same storage pool - Igroup got reused and lun got mapped
3- Storage Pool to enter maintenance mode - All Luns got unmapped and igroup also got deleted
4- Storage Pool to cancel maintenance mode - New Igroup got created with same name format and All Luns got mapped again.
5-Create a new CloudStack volume and attched to the instance - used the existing igroup and mapped the lun
6- Delete All VMs - Igroup got deleted
How did you try to break this feature and the system with this change?